I like the idea of at some point making .unwrap() do this too, because this seems unintuitive. I would be happy with either this or the prior PR (disregarding comments), as long as we plan to make the breaking change at some point.

I'm not sure it helps to provide a .unwrap_and_destroy() if we're going to end up breaking things by updating .unwrap()? The only harm is that we gain an extra method, but I'm not sure if there's benefits to this.

(or does this footgun not really matter in most cases? then maybe breaking everything isn't a good idea... sorry, I don't really know the context for this change)

@A5rocks here's the benefit of unwrap_and_destroy: https://github.com/python-trio/outcome/pull/49/changes#diff-57edcfc849c5b8867dc8c235f66164cfa33cab5f855acce0d9defa4f23bce499R115

as long as we plan to make the breaking change at some point

when I make the change in trio to use the new method I'll change the dep on outcome to outcome ~=1.4

Hmm, I admit I still don't really get it. Like, here's the way I see it (which I guess must contain something wrong)... my core assumption is that this is a footgun we'd like to remove:

1. merge this PR, release a minor version bump, update unwrap, major version bump, remove this unwrap_and_destroy, major version bump (or maybe as part of 2.0).
2. merge remove reference to value/error when unwrapping outcome, and add .peek() method #45, release a major version.

Now there's a reason to do 1: it would allow us to change Trio without breaking past versions -- until we do the major version bump. However, given we do the major version anyways, anyone on an existing version of Trio is technically using a version of outcome w/ breaking changes.

It may be valuable to have the extra few versions on Trio with a unwrap_and_destroy... except it sounds like we don't need that, and that current Trio is compatible with the breaking version of unwrap? So basically, I don't see any extra use to Trio for the extra effort 1 requires over 2. Maybe there's other affected apps we care about? (though, either way we're doing the major change so those affected apps will be affected anyways)

Personally I think this is a work-around for a bug in CPython (and pypy) in genobject.c's send/throw method and I'm leaving test_unwrap_leaves_a_refcycle as a canary test to see if this is ever fixed in python implementations

So I don't think it's so bad to have two different unwrap methods

Sorry for the delay. I hadn't really processed your test case before, that's very interesting! I've been poking at it, but nothing to report yet (it just seems that the outcome is kept alive for some reason, who knows why!). I'll try poking at it some more and see if I can find anything!

I agree that the proper fix for that outcome is to kill the outcome (i.e. by making whatever is holding it... not...), rather than break the link between it and the exception. Not sure how I feel about a patch in outcome as a work-around (that users have to apply, in addition!). But it can't hurt, I suppose.

I was under the assumption we also wanted these changes because maybe the traceback would point to the outcome! But I can't actually find examples of that in Trio, so I guess that doesn't matter?

Hmm, I was convinced by the Claude output you shared on Gitter that this was an issue because CPython isn't stealing a reference, but... I don't get it, because I can't see where the INCREF/DECREF macros are for gen_send? But it's definitely happening, cause this:

-snip-

edit: nvm it was just in another file through PyStackRef_FromPyObjectNew. Bit strange, because I don't see why this needs to be?

edit2: ok, this just happens for all Python->C->Python things, I think. Maybe other stuff too! But I reproduced using operator.call. That seems pretty general, so now I'm more of a +1 on this!